feat: opt-in v2 Argon2+HKDF encryption for multisig and MPC flows#8592
feat: opt-in v2 Argon2+HKDF encryption for multisig and MPC flows#8592pranavjain97 wants to merge 10 commits intomasterfrom
Conversation
|
Wallet sharing to be done as a separate ticket. |
pranavjain97
changed the base branch from
pranavjain/wcn-31-phase-1-hkdf-caching-layer-for-multi-call-operations
to
master
pranavjain97
force-pushed
the
pranavjain/wcn-32-v2-encryption-call-sites
branch
from
48cfa0a to
36c78cd
Compare
zahin-mohammad
left a comment
zahin-mohammad
left a comment
There was a problem hiding this comment.
Review Summary
After multiple validation rounds with parallel reviewers (architect, crypto, call-site tracer, test-gap, devil's-advocate, consensus-checker), REQUEST CHANGES with two blockers and a focused set of inline comments.
Blockers
B1. No breaking-change marker on commits. None of the 7 commits on this PR carry feat!:, fix!:, or a BREAKING CHANGE: footer. lerna publish --conventional-commits (Angular preset) requires one of those for a major bump. Result: @bitgo/sdk-core will publish a minor bump despite IWallet.getUserPrv and decryptKeychainPrivateKey changing from sync string returns to Promise<string>. Downstream JS / non-strict-TS consumers will silently get a Promise where they expect a string. The repo's Check breaking changes CI job only diffs OpenAPI specs from modules/express — it does not guard SDK type signatures, so this is a code-review responsibility entirely. Fix: add a BREAKING CHANGE: footer to commit 36c78cd (or a follow-up commit) describing the async return-type change so lerna bumps sdk-core to a major version on publish.
B2. Sync v1 decrypt fan-out callers not converted, no follow-up tickets linked. A v2-encrypted wallet that flows into any of these sites hard-throws with a confusing SJCL tag-mismatch error. Blast radius is bounded today (v2 is opt-in), but every client that opts into v2 will trip at least one of these in normal usage:
modules/sdk-core/src/bitgo/baseCoin/baseCoin.ts:647—assertIsValidKeycallssjcl.decryptdirectlymodules/abstract-utxo/src/recovery/crossChainRecovery.ts:316— syncdecrypt(passphrase, encryptedPrv)modules/sdk-core/src/bitgo/recovery/initiate.ts:112—bitgo.decrypt(affects EOS/TRX/STX/XRP/UTXO backup-key recovery)modules/sdk-core/src/bitgo/utils/tss/ecdsa/ecdsa.ts:567, 601— non-MPCv2 ECDSA offline signing roundsmodules/sdk-core/src/bitgo/keychain/keychains.ts:170—updateSingleKeychainPasswordmodules/sdk-core/src/bitgo/keychain/keychains.ts:354-364—recreateMpc(three sync decrypts)modules/sdk-core/src/bitgo/trading/tradingAccount.ts:72— OFC payload signingmodules/sdk-core/src/bitgo/wallet/wallets.ts(acceptShare / reshareWalletWithSpenders) — explicitly deferred per the PR comment
Fix options (any one): (a) convert each path to decryptAsync in this PR; (b) link follow-up tickets in the PR description for each path; (c) add a v2-opt-in guard at wallet creation that fails fast on these callers with a clear "v2 wallet operations require WCN-XX" error rather than letting SJCL surface a misleading tag-mismatch.
Major findings
See inline comments below.
Test coverage gap (not blocking, but please address)
The useV2 dispatch branches in createOfflineRound{1,2,3}Share (MPCv2) and getUserToBitgoCommitment / decryptRShare (EdDSA) currently have zero unit-test coverage. CI is green on the v2 dispatch only because no test exercises the branch — the live testnet runs in the PR description don't substitute for unit-level branch coverage. Please add at least one v2 signing-path test per coin family.
Separately: modules/bitgo/test/unit/decryptKeychain.ts (5 sync call sites) was not updated for the new async signature. The test runner uses tsx (no type-check), so this is not a CI break — but the assertions now run against a Promise object. Either the file silently fails to load and is skipped, or it asserts against a Promise and that's masked by the cdn=true reporter option. Either way the file no longer validates the function. Please add await and make the test functions async.
What was retracted from earlier review rounds
For the record:
- "CI will fail to compile" — wrong;
tsxstrips types. The decryptKeychain test file is a silent no-op, not a build break. - "MPCv2 signing creates 3× Argon2id per signing" — wrong; each round is a separate HTTP request lifecycle, so cross-round session sharing is structurally impossible without server-side session storage.
- The v2
adatadrop is documented design intent per commit22bab61c(AES-GCM is self-authenticating). The narrower remaining concern (cross-round replay binding) is captured in the inline comment onecdsaMPCv2.tsbelow.
Nits
isV2Envelopeis duplicated verbatim inecdsaMPCv2.tsandeddsa.ts— extract to a shared util.decryptAsyncfalls through to v1 SJCL on unknown future versions (v: 3+). Should fail closed withError('unknown envelope version').- No JSDoc on
EncryptOptions.adatawarning that it's silently dropped whenencryptionVersion: 2.
Great findings. Addressed all review comments across 3 commits:
|
…ase/BitGoAPI WCN-32: Adds async encryption dispatch (v1/v2 based on encryptionVersion param) and session-based encryption to the BitGoBase interface and BitGoAPI implementation. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…hain types WCN-32: Thread encryptionVersion?: 2 through GenerateWalletOptions, GenerateMpcWalletOptions, CreateMpcOptions, CreateBackupOptions, and both Lightning/GoAccount codecs. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…tion WCN-32: Convert sync encrypt() to async encryptAsync() in wallet generation and keychain creation paths. Thread encryptionVersion from GenerateWalletOptions through Lightning, GoAccount, TSS, and onchain multisig flows. Default remains v1. Only opt-in encryptionVersion: 2 triggers v2 encryption. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…n and signing WCN-32: DKLS keygen uses encryption session when encryptionVersion: 2, signing rounds auto-detect v2 from envelope and use decryptAsync/session. validateAdata skipped for v2 envelopes. All v1 paths unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
… signing WCN-32: EdDSA keygen uses encryption session when encryptionVersion: 2, signing auto-detects v2 from envelope and uses decryptAsync/session. All v1 paths unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
WCN-32: Verify that createKeychains with encryptionVersion: 2 produces v2 envelopes for encryptedPrv/reducedEncryptedPrv and that they are decryptable via decryptAsync. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…etection WCN-32: Convert decryptKeychainPrivateKey to use decryptAsync internally so signing flows work with both v1 and v2 encrypted keychains. Make getUserPrv async and update all callers across sdk-core, abstract-utxo, abstract-eth, and bitgo. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- add EncryptionVersion type alias and IEncryptionSession interface - restore sync getUserPrv/decryptKeychainPrivateKey, add async variants - share encryption session in EdDSA keygen (saves one Argon2id derivation) - extract isV2Envelope to shared util in baseTypes - fail closed on unknown envelope version in decryptAsync - remove encryptionVersion from GG18 ECDSA (not in scope, tracked in WCN-283) - fix decryptKeychain tests and add async test suite WCN-32
…ntext binding - add optional adata param to aesGcmEncrypt/aesGcmDecrypt (GCM additionalData) - store adata in v2 envelope, use as GCM AAD on encrypt and decrypt - add adata param to EncryptionSession.encrypt and IEncryptionSession - forward adata from encryptAsync to encryptV2 for v2 path - DKLS signing: pass adata to session.encrypt, call validateAdata on v2 decrypt - fix typesEddsaMPCv2 imports (renamed EddsaMPCv2KeyGen* to MPCv2KeyGen*) - add AAD round-trip, tamper detection, and session adata tests WCN-32
pranavjain97
force-pushed
the
pranavjain/wcn-32-v2-encryption-call-sites
branch
from
178ec63 to
3d25190
Compare
… with adata - e2e 3-round offline signing with v2 encrypted keys and adata context binding - validates v2 envelopes with adata at each round boundary (round 1->2->3) - tests validateAdata rejects mismatched adata on v2 envelopes - fix typesEddsaMPCv2 imports (EddsaMPCv2KeyGen* -> MPCv2KeyGen*) WCN-32
pranavjain97
force-pushed
the
pranavjain/wcn-32-v2-encryption-call-sites
branch
from
3d25190 to
4ae4ec3
Compare
2 participants
Summary
Wire v2 encryption (Argon2id + AES-256-GCM + HKDF session caching) into wallet creation and signing call sites across multisig, DKLS MPCv2, and EdDSA flows.
encryptionVersion: 2on wallet/key creationdecryptKeychainPrivateKeymade async to support v1/v2 auto-detection in signing pathsLive Node.JS Testing (testnet)
All flows tested end-to-end on testnet with real transactions:
v2 wallet creation is consistently faster due to HKDF session caching. Signing is roughly equivalent -- network round trips dominate.
Test plan
TICKET: WCN-32